-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix - test_feature_activation_loaded_programs_recompilation_phase()
#35299
Fix - test_feature_activation_loaded_programs_recompilation_phase()
#35299
Conversation
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35299 +/- ##
========================================
Coverage 81.7% 81.8%
========================================
Files 834 834
Lines 224299 224852 +553
========================================
+ Hits 183361 183937 +576
+ Misses 40938 40915 -23 |
Looks good to me. Let's also give @alessandrod a chance to take a look before merging it. |
Also, is it needed on v1.18? We are limiting backports to a minimum, but given this is a test-only modification, maybe its ok. |
Given that a bunch of RBPF feature activations are queue up and will happen when v1.18 is on MNB, yes I would like the test fixed there as well. |
This test was passing before without running the recompilation phase, it's passing now with the recompilation phase (presumably!) running, but we haven't added any new asserts so we're not testing that it's actually running? |
…o trigger the recompilation phase before the epoch boundary.
a72a4dc
to
4dbf912
Compare
4dbf912
to
d2e801a
Compare
@alessandrod I added more assertions that directly test for the presence of entries with the current / upcoming environment. Better now? |
@alessandrod can you please re-review? |
Problem
The test currently does skip most of the epoch so the recompilation phase only starts after the epoch boundary. Also, it does only have a single slot after the epoch boundary which does only select programs for recompilation but never recompiles a selected program, as that would take another slot.
Summary of Changes
Insert two slots mid epoch to start the recompilation phase and recompile the program, both before the epoch boundary.